feat: implement form data#163
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Decision: REQUEST_CHANGES
Your app is close, but several critical blockers prevent the required end-to-end flow from working and violate explicit checklist items.
Critical issues to fix (blockers)
-
Route mismatch prevents submissions from reaching the handler (Checklist 1.2, 3.2)
- Where:
src/index.html(formaction="/submit-expense") vssrc/createServer.js(expects POST'/add-expense'). - Why: Browser POST to
/submit-expensewill not hit your server’s/add-expenseroute. - How: Make them match. E.g., change the form to
action="/add-expense"or change the server route to'/submit-expense'.
- Where:
-
Body parsing incompatible with browser form (Checklist 3.3)
- Where:
src/createServer.jsusesJSON.parse(raw);src/index.htmlusesenctype="multipart/form-data". - Why: Standard forms submit
application/x-www-form-urlencoded(or multipart), not JSON;JSON.parsewill throw. - How: Preferred: remove
enctypeso the browser sendsapplication/x-www-form-urlencoded, inspectreq.headers['content-type'], and parse urlencoded viaObject.fromEntries(new URLSearchParams(raw)). Only JSON.parse whencontent-typeisapplication/json.
- Where:
-
Missing directory creation for
db/(Checklist 3.4)- Where:
src/createServer.jssetsconst file = path.resolve('db','expense.json')and writes without ensuring the directory exists. - Why: On a fresh repo, write will fail because
db/is missing. - How: Before writing, call
fs.mkdirSync(path.dirname(file), { recursive: true })(or async equivalent) and handle errors.
- Where:
-
Response must be HTML with pretty-printed JSON (Checklist 1.5, 2.2, 3.5, 3.6)
- Where:
src/createServer.jsresponds withContent-Type: application/jsonand compact JSON. - Why: Requirement: return an HTML page containing pretty-printed JSON.
- How: Use
const pretty = JSON.stringify(expense, null, 2); save pretty JSON to file; respond withContent-Type: text/htmland embed<pre>${escapedPretty}</pre>in a minimal HTML document.
- Where:
-
Insufficient validation and HTML error handling (Checklist 3.7–3.10)
- Where:
src/createServer.jsonly truthy-checks fields and returns plain text errors. - Why: Must validate that
amountis numeric anddateis parseable; on failure, return an HTML error page with appropriate status. - How:
const amount = parseFloat(payload.amount); if (Number.isNaN(amount)) ...; validate date with!isNaN(new Date(payload.date).getTime()); respond withContent-Type: text/htmlcontaining a friendly error message.
- Where:
Minor improvements (non-blockers)
- Set
Content-Type: text/htmlon GET/before pipingindex.html. - In
src/app.js, optionally useprocess.env.PORT || 5701and a listen callback to log startup.
You’ve got a solid structure; fixing the route alignment, parsing, validation, directory creation, and HTML response will make it fully compliant. Happy to provide a small patch if you’d like.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| console.log(`Server is running on http://localhost:${5701} 🚀`); | ||
| console.log('Available at http://localhost:5701'); | ||
| }); | ||
| createServer().listen(5701); |
There was a problem hiding this comment.
Consider adding a listen callback and/or using an environment variable for the port so the startup is observable and configurable. For example, you could use const PORT = process.env.PORT || 5701; createServer().listen(PORT, () => console.log(Server listening on ${PORT})); — this is optional but helpful for debugging and deployments.
src/createServer.js
Outdated
| return; | ||
| } | ||
|
|
||
| if (req.url !== '/add-expense') { |
There was a problem hiding this comment.
POST route mismatch with the form in index.html. The server expects POST to /add-expense but src/index.html posts to /submit-expense. Update either the form action or this route so they match; otherwise form submissions will not be handled.
src/createServer.js
Outdated
| let payload; | ||
|
|
||
| try { | ||
| payload = raw ? JSON.parse(raw) : {}; |
There was a problem hiding this comment.
The code uses JSON.parse on the request body. Browser form submissions (without JS) use application/x-www-form-urlencoded or multipart/form-data, not JSON; JSON.parse will throw for typical form payloads. Either change the client to send JSON, or (preferably) parse urlencoded bodies (e.g., use Object.fromEntries(new URLSearchParams(raw))) after checking req.headers['content-type']. This change is required so POST handling works with the provided index.html.
| let raw = ''; | ||
|
|
||
| req.setEncoding('utf8'); |
There was a problem hiding this comment.
There is no Content-Type inspection before parsing the body. Inspect req.headers['content-type'] and branch parsing logic: JSON.parse only for application/json; use URLSearchParams or querystring.parse for application/x-www-form-urlencoded; do not attempt to parse multipart/form-data unless you implement a parser or remove the enctype on the form. This will make parsing robust.
| return; | ||
| } | ||
|
|
||
| const file = path.resolve('db', 'expense.json'); |
There was a problem hiding this comment.
The code sets const file = path.resolve('db', 'expense.json') but never ensures the db directory exists. If db is missing, create it first (e.g., fs.mkdirSync(path.dirname(file), { recursive: true }) or the async equivalent) before creating a write stream. Without this, writing will fail on a fresh repo.
| } catch (parseErr) { | ||
| res.statusCode = 400; | ||
| res.setHeader('Content-Type', 'text/plain'); | ||
| res.end('Bad Request'); |
There was a problem hiding this comment.
Error branches currently respond with text/plain (e.g., parse error, missing fields, internal server error). Per checklist, return HTML error pages for validation and save failures so the client receives readable messages in the browser. Keep appropriate status codes, but use Content-Type: text/html and a minimal HTML body.
src/index.html
Outdated
| <title>Document</title> | ||
| </head> | ||
| <body> | ||
| <form action="/submit-expense" method="POST" enctype="multipart/form-data"> |
There was a problem hiding this comment.
Form action and enctype are incorrect for the current server. The form posts to /submit-expense and uses enctype="multipart/form-data", but the server expects POST to /add-expense and does not parse multipart form data. Update the form action to match the server route (or change the server route to /submit-expense) and remove the enctype unless you implement multipart parsing on the server. This mismatch will prevent form submissions from being handled.
src/index.html
Outdated
| <title>Document</title> | ||
| </head> | ||
| <body> | ||
| <form action="/submit-expense" method="POST" enctype="multipart/form-data"> |
There was a problem hiding this comment.
Method is correct (POST). Keep method="POST" on the form — that is required. Ensure the server route and body parsing are aligned so the POST actually reaches a handler.
| <input type="date" id="date" name="date" required /><br /><br /> | ||
|
|
||
| <label for="title">Name:</label> | ||
| <input type="text" id="title" name="title" required /><br /><br /> | ||
|
|
||
| <label for="amount">Sum:</label> | ||
| <input | ||
| type="number" | ||
| id="amount" | ||
| name="amount" | ||
| step="0.01" | ||
| required |
There was a problem hiding this comment.
Inputs have the correct name attributes: name="date", name="title", and name="amount". This matches the fields the server should extract. Good. Note: client-side required attributes help UX, but server-side validation of date parseability and numeric amount is still necessary.
| <input | ||
| type="number" | ||
| id="amount" | ||
| name="amount" | ||
| step="0.01" |
There was a problem hiding this comment.
type="date" and type="number" are helpful for client-side validation, but ensure the server validates these values too (e.g., check !isNaN(new Date(date).getTime()) for date and !Number.isNaN(parseFloat(amount)) for amount) before saving to db/expense.json. The task requires server-side validation.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Decision: REQUEST_CHANGES — there is a critical, blocking requirement missing.
Why: Requirement 2.4 mandates setting an HTML Content-Type for the form page. In src/createServer.js, the GET handler that serves index.html does not set Content-Type: text/html; charset=utf-8 (nor an explicit 200). Tests or consumers relying on this header will fail, so this is a blocker.
Critical issues to fix (with specifics and how):
- Missing Content-Type on GET form response (Requirement 2.4)
- Location:
src/createServer.js, in the GET/handler wherefs.createReadStream('src/index.html')is piped tores. - Why it’s wrong: The response lacks
text/html; charset=utf-8, violating the checklist and potentially causing test failures or incorrect rendering. - How to fix: Before piping, set
res.statusCode = 200;andres.setHeader('Content-Type', 'text/html; charset=utf-8');. Also attach a read-stream error handler to return a friendly HTML 500 ifindex.htmlis missing/unreadable, e.g.form.on('error', (err) => { console.error('Read stream error:', err); if (!res.headersSent) { res.statusCode = 500; res.setHeader('Content-Type', 'text/html; charset=utf-8'); res.end('<!doctype html>...'); } else { res.destroy(); } });, thenform.pipe(res).
- Location:
Non-blocking improvements (recommended but optional for approval):
src/createServer.js: Guard writer error handling withif (!res.headersSent)before setting headers; otherwise, log andres.destroy()or end safely.src/createServer.js: Replaceserver.on('error', () => {})withserver.on('error', (err) => console.error('Server error:', err))to avoid swallowing errors.src/app.js: Use environment-configurable port and a listen callback:const port = process.env.PORT || 5701; server.listen(port, () => console.log( Listening on ${port} ));.
You’ve done a solid job on parsing, validation, directory creation, pretty-printing, and the POST HTML response headers. Address the GET Content-Type and read error handling, and this will be ready to approve. Keep it up!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| console.log(`Server is running on http://localhost:${5701} 🚀`); | ||
| console.log('Available at http://localhost:5701'); | ||
| }); | ||
| createServer().listen(5701); |
There was a problem hiding this comment.
Consider using an environment-configurable port and a listen callback for clarity. For example, use const port = process.env.PORT || 5701 and createServer().listen(port, () => console.log(Listening on ${port})) so the port can be overridden during testing/deployment and you get a startup log.
| const form = createReadStream(path.join(__dirname, 'index.html')); | ||
|
|
||
| res.on('close', () => { | ||
| form.destroy(); | ||
| }); | ||
|
|
||
| form.pipe(res); |
There was a problem hiding this comment.
The GET handler creates a read stream for index.html and pipes it to the response but does not set the Content-Type header nor an explicit 200 status. Requirement 2.4 requires setting text/html for the form page. Also add an error listener on the read stream to handle missing/unreadable index.html and return a friendly HTML error page.
Consider adding before piping:
res.statusCode = 200;
res.setHeader('Content-Type', 'text/html; charset=utf-8');
form.on('error', (err) => {
console.error('Read stream error:', err);
if (!res.headersSent) {
res.statusCode = 500;
res.setHeader('Content-Type', 'text/html; charset=utf-8');
res.end('<!doctype html><html><head><meta charset="utf-8"><title>Error</title></head><body><h1>Server Error</h1><p>Unable to load the form.</p></body></html>');
} else {
res.destroy();
}
});
Then `form.pipe(res);` as you have.This addresses the missing Content-Type for the form page and handles read errors gracefully.
| res.statusCode = 404; | ||
| res.setHeader('Content-Type', 'text/plain'); | ||
| res.end('Not Found'); |
There was a problem hiding this comment.
The GET branch returns a 404 with Content-Type 'text/plain' for unknown URLs. It's not required to change this, but for consistency you may want to return an HTML 404 page (text/html; charset=utf-8) so browser users see a friendly page. If you keep it as plain text that's acceptable for the task, but ensure the form page itself is served with text/html.
| writer.on('error', (error) => { | ||
| console.error('Write stream error:', error); | ||
| res.statusCode = 500; | ||
| res.setHeader('Content-Type', 'text/plain'); | ||
| res.end('Internal Server Error'); |
There was a problem hiding this comment.
The writer error handler sets headers and ends the response on write-stream errors. If the response has already begun, setting headers will throw. Consider checking if (!res.headersSent) before setting headers and ending the response, or otherwise handling the case where headers are already sent (for example, destroy the connection). This prevents potential crashes or unhandled exceptions when write errors occur after response start.
| } | ||
| }); | ||
|
|
||
| server.on('error', () => {}); |
There was a problem hiding this comment.
server.on('error', () => {}) currently swallows server-level errors. At minimum log the error so issues are visible during development (and optionally handle fatal errors). For example:
server.on('error', (err) => {
console.error('Server error:', err);
});This will help debugging if the server fails to bind to a port or encounters other system-level errors.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Educational approval: You’ve met the core requirements well, and I’m approving so you can keep progressing. Great job ensuring GET serves HTML with Content-Type: text/html; charset=utf-8 and status 200, validating inputs, creating db/expense.json, and returning pretty-printed JSON in an HTML page.
Critical improvements to consider next:
- src/createServer.js line ~198: Replace
server.on('error', () => {})with logging to avoid hiding issues, e.g.,server.on('error', (err) => console.error('Server error:', err));. - src/createServer.js lines ~149–154: In
writer.on('error'), guard withif (!res.headersSent)before setting headers; otherwiseres.destroy()to prevent header-setting after send. - src/createServer.js lines ~30–37: On read-stream errors for GET, send a small HTML error body (keep the existing
!res.headersSentcheck) so clients/tests see a clear 500 page.
Minor, nice-to-have improvements:
- src/app.js: Make the port configurable (
const port = process.env.PORT || 5701) and add a listen callback log for observability. - src/index.html line ~13: Change the label text from “Name:” to “Title:” for clarity; update the
<title>fromDocumentto something descriptive likeAdd Expense.
You’re on the right track—solid functionality and thoughtful error handling patterns. Tightening the logging and headersSent guards will make your server more robust and easier to debug. Keep it up!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| console.log(`Server is running on http://localhost:${5701} 🚀`); | ||
| console.log('Available at http://localhost:5701'); | ||
| }); | ||
| createServer().listen(5701); |
There was a problem hiding this comment.
Consider making the port configurable and adding a listen callback/logging. For example:
const port = process.env.PORT || 5701;
createServer().listen(port, () => console.log(`Listening on ${port}`));This is non-blocking — the current hard-coded port works — but it improves flexibility and observability when running in different environments.
| res.statusCode = 200; | ||
| res.setHeader('Content-Type', 'text/html; charset=utf-8'); |
There was a problem hiding this comment.
Good: the GET handler explicitly sets status 200 and Content-Type to text/html; charset=utf-8, ensuring the form is served as HTML. This resolves the earlier blocker about missing Content-Type and makes the form page consistent for browsers and tests.
| if (!res.headersSent) { | ||
| res.statusCode = 500; | ||
| res.setHeader('Content-Type', 'text/html; charset=utf-8'); | ||
|
|
||
| res.end(); |
There was a problem hiding this comment.
The read-stream error handler checks whether headers were already sent, which is good. However, when responding with a 500 it currently calls res.end() with an empty body (lines 33–37). Consider returning a small HTML error page (since GET serves HTML) to make errors clearer to clients and tests. Example: res.end('<!doctype html><html>...<h1>Internal Server Error</h1></html>').
| const form = createReadStream(path.join(__dirname, 'index.html')); | ||
|
|
||
| form.on('error', (err) => { | ||
| console.error('Read stream error:', err); | ||
|
|
||
| if (!res.headersSent) { | ||
| res.statusCode = 500; | ||
| res.setHeader('Content-Type', 'text/html; charset=utf-8'); | ||
|
|
||
| res.end(); | ||
| } else { | ||
| res.destroy(); | ||
| } | ||
| }); | ||
|
|
||
| res.on('close', () => { | ||
| form.destroy(); | ||
| }); | ||
|
|
||
| form.pipe(res); |
There was a problem hiding this comment.
When creating the read stream and piping (createReadStream / form.pipe(res)), the code destroys the read stream when the response closes — good resource handling (lines 28, 43–47).
| const file = path.resolve('db', 'expense.json'); | ||
|
|
||
| let raw = ''; | ||
|
|
||
| req.setEncoding('utf8'); | ||
|
|
||
| req.on('data', (chunk) => { | ||
| raw += chunk; | ||
| }); | ||
|
|
||
| req.on('end', () => { | ||
| const contentType = req.headers['content-type'] || ''; | ||
| let payload; | ||
|
|
||
| try { | ||
| if (contentType.includes('application/json')) { | ||
| payload = raw ? JSON.parse(raw) : {}; | ||
| } else if (contentType.includes('application/x-www-form-urlencoded')) { | ||
| const params = new URLSearchParams(raw); | ||
|
|
||
| payload = Object.fromEntries(params.entries()); | ||
| } else { | ||
| payload = {}; | ||
| } | ||
| } catch (parseErr) { | ||
| res.statusCode = 400; | ||
| res.setHeader('Content-Type', 'text/plain'); | ||
| res.end('Bad Request'); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| const expense = { | ||
| date: payload.date, | ||
| title: payload.title, | ||
| amount: payload.amount, | ||
| }; | ||
|
|
||
| // Validation | ||
| const parsedAmount = | ||
| typeof expense.amount === 'string' | ||
| ? parseFloat(expense.amount) | ||
| : Number(expense.amount); | ||
| const parsedDate = new Date(expense.date); | ||
|
|
||
| const hasAll = Boolean(expense.date && expense.title && expense.amount); | ||
| const amountValid = | ||
| !Number.isNaN(parsedAmount) && Number.isFinite(parsedAmount); | ||
| const dateValid = !Number.isNaN(parsedDate.getTime()); | ||
|
|
||
| if (!hasAll || !amountValid || !dateValid) { | ||
| if (contentType.includes('application/json')) { | ||
| res.statusCode = 400; | ||
| res.setHeader('Content-Type', 'text/plain'); | ||
| res.end('Invalid payload'); | ||
| } else { | ||
| res.statusCode = 400; | ||
| res.setHeader('Content-Type', 'text/html; charset=utf-8'); | ||
|
|
||
| res.end( | ||
| `<!doctype html><html><head><meta charset="utf-8"><title>Error</title></head><body><h1>Invalid input</h1><p>Please provide a valid date, title, and numeric amount.</p></body></html>`, | ||
| ); | ||
| } | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| const normalizedExpense = { | ||
| date: expense.date, | ||
| title: expense.title, | ||
| amount: String(expense.amount), | ||
| }; | ||
|
|
||
| try { | ||
| mkdirSync(path.dirname(file), { recursive: true }); |
There was a problem hiding this comment.
Path to the data file uses path.resolve('db', 'expense.json'), which matches the requirement to persist to db/expense.json. The directory is created with mkdirSync(path.dirname(file), { recursive: true }) before writing — this satisfies the persistence requirement (lines 63 and 136–137).
| <title>Document</title> | ||
| </head> | ||
| <body> | ||
| <form action="/add-expense" method="POST"> |
There was a problem hiding this comment.
Form action and method are correct: action="/add-expense" method="POST", which matches the server POST handler and will send application/x-www-form-urlencoded data that your server parses. Good.
| <label for="date">Date:</label> | ||
| <input type="date" id="date" name="date" required /><br /><br /> |
There was a problem hiding this comment.
The date input is present with type="date", id="date", name="date", and required. This satisfies the requirement to collect the expense date.
| <label for="title">Name:</label> | ||
| <input type="text" id="title" name="title" required /><br /><br /> |
There was a problem hiding this comment.
Minor UX suggestion: the label reads Name: but the corresponding input name is title. Consider changing the label to Title: to avoid confusion between field label and internal name. This is not required for functionality but improves clarity.
| <label for="amount">Sum:</label> | ||
| <input | ||
| type="number" | ||
| id="amount" | ||
| name="amount" | ||
| step="0.01" | ||
| required | ||
| /><br /><br /> |
There was a problem hiding this comment.
The amount input uses type="number", step="0.01", id="amount", name="amount", and required — this satisfies the requirement to capture a numeric amount and aligns with server-side parsing/validation.
| <head> | ||
| <meta charset="UTF-8" /> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1.0" /> | ||
| <title>Document</title> |
There was a problem hiding this comment.
Optional improvement: the document <title> is currently Document. Consider making it descriptive (e.g., Add Expense) to help users and accessibility tools identify the page purpose.
No description provided.